Skip to content

bundle: extract DeploymentLock interface + workspace filesystem impl#5314

Draft
shreyas-goenka wants to merge 7 commits into
mainfrom
shreyas-goenka/bundle-lock-abstraction
Draft

bundle: extract DeploymentLock interface + workspace filesystem impl#5314
shreyas-goenka wants to merge 7 commits into
mainfrom
shreyas-goenka/bundle-lock-abstraction

Conversation

@shreyas-goenka
Copy link
Copy Markdown
Contributor

@shreyas-goenka shreyas-goenka commented May 22, 2026

Summary

Pure code-movement refactor. Wraps the existing workspace-filesystem lock behind a DeploymentLock interface so a follow-up PR can ship a metadata-service-backed implementation without touching deploy / destroy / bind callers again.

What changed

  • New bundle/deploy/lock/lock.goDeploymentLock interface, Goal enum (moved from release.go), DeploymentStatus enum, and a NewDeploymentLock factory that unconditionally returns the workspace filesystem implementation.
  • New bundle/deploy/lock/workspace_filesystem.goworkspaceFilesystemLock implements DeploymentLock. Preserves the historical behavior of the deleted mutators: lock-disabled short-circuit, locker.CreateLocker initialization, the permissions.ReportPossiblePermissionDenied branch on fs.ErrPermission / fs.ErrNotExist, and the destroy-mode locker.AllowLockFileNotExist unlock quirk.
  • Deleted bundle/deploy/lock/acquire.go and bundle/deploy/lock/release.go.
  • Updated bundle/phases/{deploy,destroy,bind}.go to construct the lock once via NewDeploymentLock and call Acquire / Release directly instead of via bundle.ApplyContext. The deferred Release now passes DeploymentSuccess / DeploymentFailure based on logdiag.HasError(ctx) so a future DMS-backed implementation can record the outcome.

Test plan

  • go build ./... clean
  • go vet ./bundle/deploy/lock/... ./bundle/phases/... clean
  • go test ./bundle/... ./cmd/... green
  • Lock-related acceptance goldens pass without -update: pipelines/{deploy,destroy}/force-lock, bundle/help/bundle-{deploy,deployment,deployment-migrate,destroy}
  • ./task lint clean (0 issues x3)
  • CI on databricks/cli

Pre-existing acceptance failures on origin/main (bundle/deployment/bind/job/..., bundle/templates/pydabs/check-consistency, bundle/resources/jobs/update*, etc.) reproduce identically on this branch and are unrelated to this refactor.

Pure code-movement refactor. Wraps the existing workspace-filesystem lock
behavior behind a DeploymentLock interface so a follow-up PR can introduce
an alternative metadata-service-backed lock implementation without
touching deploy/destroy/bind callers again.

What changed:
- New bundle/deploy/lock/lock.go: DeploymentLock interface, Goal enum
  (moved from release.go), DeploymentStatus enum, and a
  NewDeploymentLock factory that unconditionally returns the workspace
  filesystem implementation.
- New bundle/deploy/lock/workspace_filesystem.go: workspaceFilesystemLock
  struct that implements DeploymentLock. Preserves the historical
  behavior of the deleted acquire.go / release.go mutators: lock-disabled
  short-circuit, locker.CreateLocker initialization, the
  permissions.ReportPossiblePermissionDenied branch on fs.ErrPermission
  / fs.ErrNotExist, and the destroy-mode locker.AllowLockFileNotExist
  unlock quirk.
- Deleted bundle/deploy/lock/acquire.go and bundle/deploy/lock/release.go.
- Updated bundle/phases/{deploy,destroy,bind}.go to construct the lock
  once via NewDeploymentLock and call Acquire / Release directly instead
  of through bundle.ApplyContext. The deferred Release now reports
  DeploymentSuccess / DeploymentFailure based on logdiag.HasError so a
  future DMS-backed implementation can record the outcome.

Behavior is preserved end-to-end: lock-related acceptance goldens
(pipelines/{deploy,destroy}/force-lock, bundle/help/bundle-{deploy,
destroy}) all pass unchanged.
@eng-dev-ecosystem-bot
Copy link
Copy Markdown
Collaborator

eng-dev-ecosystem-bot commented May 22, 2026

Commit: 6cad232

Run: 26568611498

…dle.Bundle

Pre-refactor the locker was stashed on b.Locker so the two mutators
(acquire then release) could share it via the Bundle. After PR #5314
both lifecycle methods live on a single struct, so the cross-method
state-passing through bundle.Bundle is redundant — grep finds zero
external consumers of b.Locker.

Move the *locker.Locker to a field on workspaceFilesystemLock and delete
the now-unused Locker field on bundle.Bundle.

Co-authored-by: Isaac
The struct now holds only the primitives + workspace client + a permission-error
callback it needs. The bundle-aware wiring lives in NewDeploymentLock, which
captures everything from the *bundle.Bundle at construction time.

Why: keeps the type-level dependency surface narrow (the impl no longer imports
bundle or bundle/permissions), removes a class of accidental coupling that
would make alternative lock implementations awkward, and forecloses the
possibility of a future bundle <-> lock import cycle.

NewDeploymentLock now takes ctx so it can call b.WorkspaceClient(ctx) at
construction; the three callers in bundle/phases are updated in one line each.

Co-authored-by: Isaac
The lock and workspace_filesystem files live in the same package, so the
package-level import of bundle/permissions exists regardless of whether the
struct hides the call behind a function value. The callback only added a
layer of indirection; switch to a direct
permissions.ReportPossiblePermissionDenied call and keep a narrow b field
purely for that error path.

Co-authored-by: Isaac
The error is returned to the caller; logging it here just produces a
duplicate line in the user-facing output. Drop the log; preserve the
permission-denied branch and the bare return.

Co-authored-by: Isaac
…or semantics)

The original release.go mutator returned diag.FromErr on unlock failure,
which surfaced as an error diagnostic to the user. The defer pattern
introduced in #5314 dropped it to log.Warnf — a silent demotion that
would hide a stuck lock from the user (who normally has to recover with
--force-lock).

Switch the defer to logdiag.LogError so the unlock failure shows up as a
proper diagnostic, matching the pre-refactor behavior. The deploy/destroy/
bind/unbind phases all share the same fix.

Co-authored-by: Isaac
Comment thread bundle/phases/bind.go

defer func() {
bundle.ApplyContext(ctx, b, lock.Release(lock.GoalBind))
status := lock.DeploymentSuccess
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

status is not used for now - but will be used with deployment metadata service.

…lesystemLock

ReportPossiblePermissionDenied has a second caller (bundle/deploy/files/
upload.go) and walks bundle.Config.RunAs / Permissions / Resources via
analyzeBundlePermissions, so the lock can't simply inline it or call a
simplified variant. But the struct doesn't need to pin a *bundle.Bundle
field — the bundle-aware wiring can live in a callback closure that
NewDeploymentLock builds at construction time. After this commit
workspaceFilesystemLock holds only primitives, a workspace client, and
the callback; no bundle types appear in its field list.

Co-authored-by: Isaac
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants